PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

# Extract upload_extra value using yq
upload_extra=$(yq -r '.upload_extra // "none"' "$template_file" 2>/dev/null || echo "none")

# Skip if upload_extra is 'none', empty, or null
if [[ "$upload_extra" == "none" ]] || [[ -z "$upload_extra" ]] || [[ "$upload_extra" == "null" ]]; then
    continue
fi

# Check if upload_extra is an array or a string
is_array=$(yq -r '.upload_extra | type' "$template_file" 2>/dev/null || echo "null")

if [[ "$is_array" == "array" ]]; then
    # It's an array, process each element
    while IFS= read -r path; do
        if [[ "$path" != "none" ]] && [[ -n "$path" ]]; then
            if [[ ! -e "$path" ]]; then
                missing_kits+=("$template_name:$path")
            fi
        fi
    done < <(yq -r '.upload_extra[]' "$template_file" 2>/dev/null)
else

if [[ $gl_ssh_key_file == "" ]]; then
	echo "  ssh_key: $HOME/.ssh/id_rsa" >> ansible_vars_main.yml
	echo "  ssh_public_key: $HOME/.ssh/id_rsa.pub" >> ansible_vars_main.yml
else
	echo "  ssh_key: ${gl_ssh_key_file}" >> ansible_vars_main.yml
	if [[ $gl_ssh_public_key_file != "" ]]; then
		echo "  ssh_public_key: ${gl_ssh_public_key_file}" >> ansible_vars_main.yml
	fi
fi

PR Code Suggestions ✨

Explore these optional code suggestions:

 		if [[ $gl_ssh_key_file == "" ]]; then
 			echo "  ssh_key: $HOME/.ssh/id_rsa" >> ansible_vars_main.yml
 			echo "  ssh_public_key: $HOME/.ssh/id_rsa.pub" >> ansible_vars_main.yml
 		else
 			echo "  ssh_key: ${gl_ssh_key_file}" >> ansible_vars_main.yml
 			if [[ $gl_ssh_public_key_file != "" ]]; then
 				echo "  ssh_public_key: ${gl_ssh_public_key_file}" >> ansible_vars_main.yml
+			else
+				echo "  ssh_public_key: ${gl_ssh_key_file}.pub" >> ansible_vars_main.yml
 			fi
 		fi

-verify_ssh_key_file()
+verify_file_exists()
 {
-	if [[ ! -f $1 ]]; then
-		cleanup_and_exit "Error: ssh key file $1 does not exist." 1
+	local file_path="$1"
+	local file_description="$2"
+	if [[ ! -f "$file_path" ]]; then
+		cleanup_and_exit "Error: $file_description file '$file_path' does not exist." 1
 	fi
 }
 
-verify_ssh_public_key_file()
-{
-	if [[ ! -f $1 ]]; then
-		cleanup_and_exit "Error: ssh public key file $1 does not exist." 1
-	fi
-}
-

The removal of the auto-derivation (${gl_ssh_key_file}.pub) is intentional, not a flaw. Users should explicitly specify the public key path when needed for provisioning scenarios, rather than having it automatically derived.

Test Results: https://gist.github.com/sayalibhavsar/236e35ee79ec92b36ef02f71cc497a05

PullHero Review

Provider: generativelanguage.googleapis.com Model: gemini-2.0-flash

Okay, I've reviewed the changes. Here's my assessment:

Overview

This PR introduces the ability to specify a separate SSH public key file. Currently, the code assumes the public key is located at ~/.ssh/id_rsa.pub based on the private key specified, but this change allows for greater flexibility in key management by explicitly defining the public key path.

Analysis

Code Quality and Maintainability:

• The code is generally well-structured and easy to follow.
• The addition of a new global variable (gl_ssh_public_key_file) and corresponding functions (verify_ssh_public_key_file, set_general_value) is a clean and logical extension of the existing framework.
• The modifications to the Terraform template files (tfvars.j2) are straightforward and maintain consistency.

Potential Bugs or Security Issues:

• It is important to consider if the tool should verify that the public key actually corresponds to the private key. While this adds complexity, it could prevent misconfiguration where the wrong public key is used. The current code trusts that the user provides the correct key pair.
• The code adds a new parameter, but doesn't enforce its use, meaning the old behavior persists. This may or may not be desired, but should be explicitly documented.

Adherence to Project Conventions:

• The changes seem to follow the established coding style and conventions of the project.
• The introduction of the new option (--ssh_public_key_file) is consistent with the existing CLI option structure.

Documentation Completeness:

• The bin/burden usage output has been updated to include the new option, which is good.
• The docs/command_line_reference.md file needs to be updated to reflect the new command line option and explain its usage.
• The documentation should clearly state that if --ssh_public_key_file is not specified, the tool defaults to assuming the public key is located at ~/.ssh/id_rsa.pub (based on ssh_key_path).
• The documentation should discuss the security implications of using separate public and private key files.

Specific Feedback:

• Recommendation: Add a check to verify_ssh_public_key_file to ensure the file is readable and contains a valid public key format.
• Suggestion: Consider adding a warning message if only --ssh_key_file is provided, reminding the user of the default public key path assumption.
• Suggestion: Add a check in create_ansible_options() to verify if gl_ssh_public_key_file is set, if it's not, then fall back to the default assumption ssh_public_key: $HOME/.ssh/id_rsa.pub so that the ansible code can use it.
• Improvement: Update docs/command_line_reference.md to fully document the new option with examples.

Vote: -1

The code itself is good, however, documentation needs to be updated to reflect the changes. Additionally, it would be better to have a check to make sure the user is providing a valid public key file.

Vote: -1

PullHero

This relates to RPOPC-492

Want output from a uperf run; verify this does not break old scenario files, provide output

Review Summary by Qodo

Add explicit SSH public key configuration support across cloud providers

✨ Enhancement

Walkthroughs

• Add --ssh_public_key_file CLI option to bin/burden for explicit SSH public key configuration
• Introduce ssh_public_key_path Terraform variable across AWS, Azure, and GCP providers
• Replace hardcoded public key paths with configurable variable references
• Maintain backward compatibility with default ~/.ssh/id_rsa.pub path

flowchart LR
  CLI["CLI Option<br/>--ssh_public_key_file"]
  BURDEN["bin/burden<br/>Script"]
  ANSIBLE["Ansible Variables<br/>ssh_public_key"]
  TF["Terraform Variables<br/>ssh_public_key_path"]
  CLOUDS["Cloud Providers<br/>AWS/Azure/GCP"]
  
  CLI -->|"Parsed by"| BURDEN
  BURDEN -->|"Generates"| ANSIBLE
  ANSIBLE -->|"Passed to"| TF
  TF -->|"Configures"| CLOUDS

File Changes

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (1)

ⓘ The new review experience is currently in Beta. Learn more

When --ssh_key_file is provided but --ssh_public_key_file is not, the ssh_public_key variable was never written to ansible_vars_main.yml. This caused the Terraform template (tfvars.j2) to fail with: 'dict object' has no attribute 'ssh_public_key'. Now it defaults to $HOME/.ssh/id_rsa.pub — matching the behavior when no SSH key is specified at all (line 1913).

uperf with public key
uperf without public key

PullHero Review

Provider: generativelanguage.googleapis.com Model: gemini-2.0-flash

OK. Here's a review of the changes with a focus on code quality, potential issues, and adherence to project conventions.

Overview

The changes introduce a new ssh_public_key_path variable and associated logic to the Zathras framework. This allows users to explicitly specify the path to the SSH public key file, rather than relying on the default ~/.ssh/id_rsa.pub. This enhances flexibility, particularly when dealing with non-standard key locations or automated key management systems.

Code Quality and Maintainability

• Variable Definitions: The addition of ssh_public_key_path with a default value of ~/.ssh/id_rsa.pub in vars.tf for aws_create, azure_create and gcp_create_instance roles is well structured.
• Template Modifications: The modifications in tfvars.j2 and related templates to utilize ssh_public_key_path are straightforward and easy to understand.
• bin/burden Changes: The changes to bin/burden are logically organized and follow existing patterns for handling command-line arguments and setting variables. The added verify_ssh_public_key_file function is also good practice.
• Consistency: The changes are consistently applied across different cloud providers (AWS, Azure, GCP), ensuring uniform behavior.

Potential Bugs or Security Issues

• Default Public Key: The default public key path ~/.ssh/id_rsa.pub might not always be the correct one. While the addition of ssh_public_key_path provides a way to override this, it's important to clearly document that users must provide this if the public key is not in the default location.
• Key Validation: The verify_ssh_public_key_file function checks for the existence of the file, but not whether it's a valid public key. While the Terraform file() function will likely fail if the contents of the file are invalid, it would be beneficial to add a basic validation step (e.g., checking the file extension or content format) to provide earlier feedback to the user.
• Security: While not directly introduced by these changes, the reliance on SSH key-based authentication should be carefully considered in the context of overall security. Users should be encouraged to use strong, unique key pairs and to protect their private keys.

Adherence to Project Conventions

• The changes follow the existing project structure and coding style.
• The naming conventions for variables and functions are consistent with the rest of the codebase.
• The use of Terraform templates and Ansible roles aligns with the project's infrastructure-as-code approach.

Documentation Completeness

• The commit message and PR description should clearly explain the purpose of the changes and how to use the new ssh_public_key_path option.
• The documentation in docs/command_line_reference.md needs to be updated to reflect the new ssh_public_key_file option. Also, the use of the variable should be documented (e.g., in docs/testing_quickstart.md).

Suggestions

1. Add documentation: Update docs/command_line_reference.md and other relevant documentation files to describe the new ssh_public_key_file option and its usage. Emphasize the importance of specifying the correct public key path.
2. Consider Public Key Validation: Add a basic validation step in verify_ssh_public_key_file to check the file extension or content format. This could be a simple regex check.
3. Update Examples: Provide examples in the documentation and/or README of how to use the new option with different cloud providers and scenarios.

Vote

Vote: +1

Vote: +1

PullHero

PullHero Review

Provider: generativelanguage.googleapis.com Model: gemini-2.0-flash

Okay, I've reviewed the proposed changes. The primary goal of this PR seems to be to allow users to explicitly specify the public key file used for SSH authentication, rather than relying on a hardcoded default (~/.ssh/id_rsa.pub). This provides more flexibility, especially when dealing with different key pairs or when the standard location isn't used.

Analysis

• Code Quality and Maintainability: The changes are relatively straightforward and easy to understand. Adding a new variable and updating the templates to use it is a common pattern.
• Potential Bugs or Security Issues: No immediate security concerns are apparent. The changes primarily involve specifying a path to a file, not modifying the authentication logic itself. However, it is important to make sure that the path is correctly handled and that the user has appropriate permissions to read the specified public key file.
• Adherence to Project Conventions: The changes seem to fit within the existing project structure and coding style.
• Documentation Completeness: The documentation needs to be updated to reflect the new --ssh_public_key_file option. The docs/command_line_reference.md file needs to be updated, and a brief mention in the README.md file might be helpful.

Specific Feedback

1. Variable Naming: The variable names are consistent and descriptive (e.g., ssh_public_key_path). Good job.
2. Default Value: The default value for the new variables seems sensible (~/.ssh/id_rsa.pub). It maintains backward compatibility.
3. Code Duplication: There is some duplication in the admin_ssh_key block within Azure's Terraform files (main_net_p2_sub.tf, main_net_p2_urn.tf, vm_spot_set_sub.tf, vm_spot_set_urn.tf). While minor, consider creating a template snippet or a local variable to avoid this repetition.
4. Error Handling: There isn't explicit error handling for cases where the specified public key file doesn't exist or is unreadable. While the Terraform file() function should throw an error, it might be worth adding a pre-check within the ansible roles to validate the existence and readability of the file. This would provide a more user-friendly error message.
5. Documentation: The docs/command_line_reference.md file must be updated to include the new --ssh_public_key_file option, its purpose, and usage. Update README.md with this new option.

Recommendations

1. Azure Terraform Refactoring (Minor): Refactor the Azure Terraform files to reduce code duplication in the admin_ssh_key block.
2. Ansible Pre-Check (Optional): Consider adding a task in the Ansible roles to verify the existence and readability of the specified public key file.
3. Documentation Update (Mandatory): Update docs/command_line_reference.md and README.md to include the --ssh_public_key_file option.

Vote: -1

Vote: -1

PullHero